-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for displaying the version with wt --version
#5501
Conversation
if (const auto appLogic{ winrt::TerminalApp::implementation::AppLogic::Current() }) | ||
{ | ||
// Set our message to display the application name and the current version. | ||
_exitMessage = fmt::format("{0}\n{1}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yyyyaaaasssss
@@ -115,7 +115,10 @@ void AppHost::_HandleCommandlineArgs() | |||
GetStringResource(messageTitle).data(), | |||
MB_OK | messageIcon); | |||
|
|||
ExitProcess(result); | |||
if (_logic.ShouldExitEarly()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're still covered in the version
case by the message
being non-empty -- we could simplify this changeset more if there's no compelling reason to introduce ShoudlExitEarly up through the stack. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sure, we don't need it in this PR anymore. Originally I was only going to exit early if there were no subcommands specified after a --version
.
Now, I'm thinking about using it in the future for open-settings
- if there's no subcommands other than open-settings
, just open the file and exit. I don't feel strongly about keeping it around for now, but I will probably be using it in the future so ¯\_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with it as-is, but I am curious. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with keeping it to make the future easier. Looks good.
@zadjii-msft Can we merge this in? If it's got enough approvals, we should get this in for v1. |
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
🎉 Handy links: |
Summary of the Pull Request
Here's 3000 words:
References
PR Checklist